-
Notifications
You must be signed in to change notification settings - Fork 25
Optimize label handling with predefined label keys in counter/gauge #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
variable labels: 85.6 s Bench scriptlocal metrics = require('metrics')
local clock = require('clock')
local log = require('log')
local GAUGES_COUNT = 75
local KEYS_COUNT = 10000
local OPS_COUNT = 50000000
local label_keys = {
"service", "version", "env", "region", "status", "deployment"
}
local label_values = {
service = { "auth", "cart", "payment", "storage", "analytics", "notify", "gateway" },
version = { "v1.0", "v1.1", "v1.2", "v2.0", "v2.1", "v2.2", "v3.0-beta" },
env = { "prod", "stage", "dev", "test", "canary" },
region = { "eu-west", "us-east", "us-west", "asia-south", "asia-east" },
status = { "active", "inactive", "draining", "booting" },
deployment = { "blue", "green", "red", "canary" }
}
local function generate_random_labels()
local labels = {}
for _, key in ipairs(label_keys) do
local values = label_values[key]
labels[key] = values[math.random(1, #values)]
end
return labels
end
local label_combinations = {}
for _ = 1, KEYS_COUNT do
table.insert(label_combinations, generate_random_labels())
end
local var_gauges = {}
local fixed_gauges = {}
for i = 1, GAUGES_COUNT do
var_gauges[i] = metrics.gauge('var_gauge_' .. i, 'Description for gauge ' .. i)
fixed_gauges[i] = metrics.gauge('fixed_gauge_' .. i, 'Description for gauge ' .. i, {}, label_keys)
end
local function run_bench(gauges)
local start_time = clock.monotonic()
for _ = 1, OPS_COUNT do
local gauge_idx = math.random(1, #gauges)
local label_idx = math.random(1, #label_combinations)
local value = math.random(1, 1000)
gauges[gauge_idx]:set(value, label_combinations[label_idx])
end
return clock.monotonic() - start_time
end
local t1 = run_bench(var_gauges)
local t2 = run_bench(fixed_gauges)
local t3 = run_bench(var_gauges)
local t4 = run_bench(fixed_gauges)
log.info("variable labels: %.10f s", (t1 + t3) / 2)
log.info("fixed labels: %.10f s", (t2 + t4) / 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation? Is it the optimization or more assertive API for metrics
users? If it's the optimization, I think the solution can be expanded to remove redundant table allocation (parts = {}
) or even table.concat
. As far as I remember, they also take a lot of work.
We shouldn't forget to
- add a CHANGELOG entry,
- update the API documentation (like here).
metrics/collectors/shared.lua
Outdated
if type(label_pairs) ~= 'table' then | ||
return "" | ||
end | ||
|
||
if label_keys ~= nil then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be
if type(label_pairs) ~= 'table' then | |
return "" | |
end | |
if label_keys ~= nil then | |
if (label_keys == nil) and (type(label_pairs) ~= 'table') then | |
return "" | |
end | |
if label_keys ~= nil then |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If label_keys
is not nil and label_pairs
is invalid, we won't be able to make the key
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're fine with ignoring invalid label_pairs
, including nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean now
local parts = table.new(#label_keys, 0) | ||
for i, label_key in ipairs(label_keys) do | ||
local label_value = label_pairs[label_key] | ||
if label_value == nil then | ||
error(string.format("Label key '%s' is missing", label_key)) | ||
end | ||
parts[i] = label_value | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to throw error on extra label as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that would require iterating over the hash table label_pairs
, adding overhead. Not sure if it's worth it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends on whether you want to fixate a correct behavior or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check
This change adds support for predefined label keys in
counter
andgauge
metric constructors, optimizing label processing.Changes:
label_keys
table tocounter()
andgauge()
initializationlabel_keys
are provided, eliminating unnecessary processing overheadDynamic labels still supported when
label_keys
is omitted.Closes TNTP-3453